stack 4/7: keep an explicit thinking disable through translation (#545) - #954
Merged
Conversation
) Claude Desktop 3P Auto Mode sends thinking:{type:"disabled"} with max_tokens:64 and a stop sequence. Inbound translation dropped the instruction — reasoning stayed undefined, indistinguishable from a request that never mentioned thinking — so the outbound Anthropic body omitted the field entirely. For Sonnet 5 an omitted thinking field means adaptive thinking is ON, and thinking shares max_tokens, so generation ran out of budget before it could emit </block>. Claude Code then retried, up to five times per tool approval. The gate is deliberately its own predicate rather than usesAdaptiveThinking(), which answers a different question: Fable always thinks and rejects an explicit disable, while Opus 4.7/4.8 leave thinking off when the field is omitted. Widening it would trade a silent truncation for a 400. Refs #545
A modelMap entry can point at a routed destination like anthropic/claude-sonnet-5, which custom-provider routing decodes back into a slash-carrying native id. Both capability predicates anchored on ^claude-, so those requests silently missed the gate and the model thought anyway — the exact defect, just harder to see. Extracted the shared family/version parse so usesAdaptiveThinking() gets the same tolerance, and pinned all four id shapes plus a prefixed negative case. Also pins the Cursor effect: an explicit "none" now selects the lowest tier rather than the top one. Cursor has no off switch for a reasoning model, and the lowest tier is the closest honest reading of "do not think" — dropping the instruction sent these to the maximum tier, the opposite of what the caller asked for.
#545) The previous normalization took the last slash-separated segment, which fixed anthropic/claude-sonnet-5 and broke claude-sonnet-5/variant — a custom provider can expose a native id where the slash carries a vendor suffix rather than a routing prefix. That regression was worse than the bug: the adaptive-wire predicate shares this parse, so a slash-suffixed Sonnet 5 would have been sent obsolete manual thinking.enabled and 400d. Match the segment that actually begins with claude-, at either boundary, and pin both directions plus a double prefix and the adaptive-shape cases.
📝 WalkthroughWalkthroughClaude model detection now supports routed model IDs and Sonnet 5+ thinking disablement. Explicit ChangesClaude thinking control flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeRequest
participant AnthropicAdapter
participant ClaudeCapabilityParser
participant AnthropicAPI
ClaudeRequest->>AnthropicAdapter: reasoning effort
AnthropicAdapter->>ClaudeCapabilityParser: parse routed Claude model ID
ClaudeCapabilityParser-->>AnthropicAdapter: thinking disable capability
AnthropicAdapter->>AnthropicAPI: thinking disabled configuration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 3, 2026
Owner
Author
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. The layers touch disjoint files — #954 needs human security review per Carried in #953, with authorship preserved: #939, #942, #943, #944, #945, #948. |
Owner
Author
lidge-jun
changed the title
This was referenced Aug 4, 2026
Owner
Author
lidge-jun
changed the title
lidge-jun
changed the title
Owner
Author
Stack navigation — 7 layers, review and merge bottom-up
Each layer targets the branch below it, so its diff only makes sense on that base — Note for the merge sequence: retargeting a child after its parent merges emits an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/anthropic.ts`:
- Around line 427-437: Update claudeFamilyVersion to accept either a hyphen or
dot separator before the optional minor version, so IDs such as
claude-sonnet-4.5 produce minor 5 while preserving existing hyphenated parsing.
Add a dot-separated model ID regression case to the capability tests covering
meetsFamilyMinimum().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d4f0740-8646-494e-b0cc-2b9d2bd50a23
📒 Files selected for processing (5)
src/adapters/anthropic.tssrc/claude/inbound.tstests/anthropic-reasoning.test.tstests/claude-inbound.test.tstests/cursor-effort-suffix.test.ts
Comment on lines
+427
to
+437
| function claudeFamilyVersion(modelId: string): { family: string; major: number; minor: number } | undefined { | ||
| // Find the segment that actually starts with `claude-`, rather than assuming it is either | ||
| // the first (breaks `anthropic/claude-sonnet-5`) or the last (breaks `claude-sonnet-5/variant`, | ||
| // where the slash carries a vendor suffix rather than a routing prefix). | ||
| const match = /(?:^|\/)claude-([a-z]+)-(\d+)(?:-(\d{1,2}))?(?!\d)/.exec(modelId); | ||
| if (!match) return undefined; | ||
| return { | ||
| family: match[1]!, | ||
| major: Number(match[2]), | ||
| minor: match[3] === undefined ? 0 : Number(match[3]), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parse dot-separated Claude minor versions.
Line 431 parses anthropic/claude-sonnet-4.5 as version 4.0. The optional minor group only accepts -5, and the negative lookahead permits the . after 4. This makes meetsFamilyMinimum() evaluate the wrong version for model IDs already used in tests/anthropic-reasoning.test.ts.
Accept both - and . before the minor version. Add a dot-separated ID to the capability regression cases.
Proposed fix
- const match = /(?:^|\/)claude-([a-z]+)-(\d+)(?:-(\d{1,2}))?(?!\d)/.exec(modelId);
+ const match = /(?:^|\/)claude-([a-z]+)-(\d+)(?:[-.](\d{1,2}))?(?!\d)/.exec(modelId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function claudeFamilyVersion(modelId: string): { family: string; major: number; minor: number } | undefined { | |
| // Find the segment that actually starts with `claude-`, rather than assuming it is either | |
| // the first (breaks `anthropic/claude-sonnet-5`) or the last (breaks `claude-sonnet-5/variant`, | |
| // where the slash carries a vendor suffix rather than a routing prefix). | |
| const match = /(?:^|\/)claude-([a-z]+)-(\d+)(?:-(\d{1,2}))?(?!\d)/.exec(modelId); | |
| if (!match) return undefined; | |
| return { | |
| family: match[1]!, | |
| major: Number(match[2]), | |
| minor: match[3] === undefined ? 0 : Number(match[3]), | |
| }; | |
| function claudeFamilyVersion(modelId: string): { family: string; major: number; minor: number } | undefined { | |
| // Find the segment that actually starts with `claude-`, rather than assuming it is either | |
| // the first (breaks `anthropic/claude-sonnet-5`) or the last (breaks `claude-sonnet-5/variant`, | |
| // where the slash carries a vendor suffix rather than a routing prefix). | |
| const match = /(?:^|\/)claude-([a-z]+)-(\d+)(?:[-.](\d{1,2}))?(?!\d)/.exec(modelId); | |
| if (!match) return undefined; | |
| return { | |
| family: match[1]!, | |
| major: Number(match[2]), | |
| minor: match[3] === undefined ? 0 : Number(match[3]), | |
| }; |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 431-431: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/anthropic.ts` around lines 427 - 437, Update claudeFamilyVersion
to accept either a hyphen or dot separator before the optional minor version, so
IDs such as claude-sonnet-4.5 produce minor 5 while preserving existing
hyphenated parsing. Add a dot-separated model ID regression case to the
capability tests covering meetsFamilyMinimum().
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
4/4 — Claude Desktop classifier thinking round-trip
Base:
codex/carry-contributor-bugfixes(#953)Summary
Claude Desktop 3P Auto Mode sends
thinking: {type: "disabled"}withmax_tokens: 64and a</block>stop sequence. The instruction was dropped in translation, so the model thought anyway and spent the 64-token budget before it could close the tag — and Claude Code retried, up to five times per tool approval. The reporter measured 1,084 truncated requests against 143 that completed.src/claude/inbound.ts— an explicit disable is preserved as the parser's"none"sentinel instead of becomingundefinedsrc/adapters/anthropic.ts— emitthinking: {type: "disabled"}for models that both default to thinking-on and accept the explicit disableThe standing hypothesis was impossible
The recorded analysis on #545 said the prepended OAuth identity block consumed the classifier's 64 output tokens. It cannot: the identity goes into the system prompt, and
max_tokenscaps output. Different budgets. This repository had already reached that conclusion once —devlog/_fin/260728_bug_bundle_resolution/030_claude_system_dedup.mdabandoned an identity-dedup patch for exactly this reason — and I re-derived the rejected theory before testing it.The real chain is a round-trip fidelity loss:
src/claude/inbound.ts:494treateddisabledas nothing to translate, leavingreasoningundefinedtests/claude-inbound.test.tspinned both toundefinedthinkingonly for a real non-noneeffort, so the field was omitted outboundthinkingfield means adaptive thinking is ON, and thinking tokens count againstmax_tokensThe client asked for no thinking and got thinking. That also explains the shape of the reporter's data: the 143 requests that completed are the ones where thinking happened to stay short.
The gate is deliberately narrow
usesAdaptiveThinking()looks like the right predicate and is not — it answers which wire shape a family accepts, not whether omission means thinking-on. The sets differ both ways: Fable always thinks and rejects an explicit disable, while Opus 4.7/4.8 use the adaptive wire but leave thinking off when omitted. Reusing it would have required breaking a passing test (tests/anthropic-reasoning.test.tsassertsclaude-fable-5+"none"sends no thinking config) in order to ship a production 400.So
supportsExplicitThinkingDisable()is seeded withsonnet: [5,0]only. Widen it per family with vendor evidence.What the audit caught
Three rounds, two FAIL. Both findings were reproduced at runtime before fixing.
Round 1 —
anthropic/claude-sonnet-5silently missed the gate. AmodelMapentry can point at a routed destination that routing decodes back into a slash-carrying native id, and the predicate anchored on^claude-.Round 2 — my own round-1 fix was worse than the bug. Normalizing with
lastIndexOf("/")repaired the prefix case and broke the suffix case (claude-sonnet-5/variant), and because the adaptive-wire predicate shares that parse, such a model would have been sent obsolete manualthinking.enabledand 400d. A silent truncation traded for a hard failure.Both now match the segment that actually begins with
claude-, at either boundary, with all shapes pinned in the matrices — including a new adaptive-shape test covering the 400 path.Cross-provider note
A
modelMapthat routes such a request to Cursor now selects the model's lowest tier rather than its top one. Cursor has no off switch for a reasoning model, so the lowest tier is the closest honest reading of "do not think" — the previous behavior sent these to the maximum tier, the opposite of the caller's instruction. Pinned intests/cursor-effort-suffix.test.tsso it is deliberate rather than emergent.Verification
bun x tsc --noEmit— exit 0bun run test— 7711 pass, 8 skip, 0 fail, 507 filesbun run privacy:scan— passedTwo honest limits
The tests prove the wire shape, not that the retries stop. Confirming that needs a live Claude Desktop 3P + Anthropic OAuth session showing the classifier terminating on
</block>instead ofmax_tokens. I have asked the reporter rather than claiming the symptom fixed.This changes request construction on an Anthropic OAuth execution path, so
MAINTAINERS.mdrequires explicit human security review — no credential handling is touched, but the boundary is. Please do not merge on CI alone.Refs #545
Summary by CodeRabbit